Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: マナが無くなったら掘れなくするパッシブスキルを追加 #2370

Open
wants to merge 19 commits into
base: 1_18
Choose a base branch
from
Open

feat: マナが無くなったら掘れなくするパッシブスキルを追加 #2370

wants to merge 19 commits into from
+398 −13

Conversation

kuroma6666
Copy link
Contributor

@kuroma6666 kuroma6666 commented Aug 31, 2024 •
edited
Loading

close #1964

このPRの変更点と理由:

主な変更点

  • パッシブスキルに対して、スキルをONにしている際にマナを消費しきった際に整地スキルによってブロック破壊しない設定を追加

補足情報:

  • デフォルト設定値はOFF設定とする（初心者プレイヤー詰み防止）
  • パッシブスキル設定は永続化することとしました

@kuroma6666 kuroma6666 force-pushed the feat-digging-stop-mana-fully-consumed branch 2 times, most recently from 14a859f to 571abd3 Compare September 14, 2024 17:30
@kuroma6666 kuroma6666 marked this pull request as ready for review September 14, 2024 17:38
@kuroma6666
Copy link
Contributor Author

@rito528
ご確認よろしくお願いします。

rito528
rito528 requested changes Sep 15, 2024
View reviewed changes
Copy link
Member

@rito528 rito528 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

develop ブランチの変更を取り込んでしまっているようなので、 git rebase --rebase-merges 1_18 などを実行して develop ブランチの変更をコミット履歴から消していただけると助かります🙇‍♂️🙇‍♂️

@kuroma6666
Copy link
Contributor Author

了解です。

@kuroma6666 kuroma6666 force-pushed the feat-digging-stop-mana-fully-consumed branch 2 times, most recently from bb09299 to 509644b Compare September 15, 2024 06:27
@kuroma6666 kuroma6666 force-pushed the feat-digging-stop-mana-fully-consumed branch from 509644b to e7c7cd3 Compare September 15, 2024 06:46
@kuroma6666
Copy link
Contributor Author

@rito528
developブランチの変更を取り込んでしまっていた内容については削除しました。

rito528
rito528 requested changes Sep 15, 2024
View reviewed changes
Copy link
Member

@rito528 rito528 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

一旦2つ変更をお願いします

src/main/scala/com/github/unchama/seichiassist/listener/PlayerBlockBreakListener.scala Outdated Show resolved Hide resolved
src/main/scala/com/github/unchama/seichiassist/util/BreakUtil.scala Outdated Show resolved Hide resolved
@kuroma6666
Copy link
Contributor Author

kuroma6666 commented Sep 21, 2024 •
edited
Loading

一旦2つ変更をお願いします

@rito528
一旦変更かけました

@kuroma6666
Copy link
Contributor Author

@rito528
レビュー内容について対応実施しました、再確認お願いします。

rito528
rito528 requested changes Sep 22, 2024
View reviewed changes
Copy link
Member

@rito528 rito528 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

テーブル構成が変わることで今と実装がかなり変わりそうなので、一度以下の内容の変更をお願いします！

src/main/scala/com/github/unchama/seichiassist/subsystems/mana/domain/ManaManipulation.scala Outdated Show resolved Hide resolved
src/main/scala/com/github/unchama/seichiassist/listener/PlayerBlockBreakListener.scala Outdated Show resolved Hide resolved
src/main/scala/com/github/unchama/seichiassist/listener/PlayerBlockBreakListener.scala Outdated Show resolved Hide resolved
src/main/resources/db/migration/V1.19.0__Create_break_trigger_preference_table.sql Outdated Show resolved Hide resolved
@kuroma6666
Copy link
Contributor Author

テーブル構成が変わることで今と実装がかなり変わりそうなので、一度以下の内容の変更をお願いします！

@rito528
一旦変更をかけたので、確認お願いします。

rito528
rito528 requested changes Oct 12, 2024
View reviewed changes
...unchama/seichiassist/subsystems/breakskilltriggerconfig/domain/BreakSkillTriggerConfig.scala
@@ -0,0 +1,28 @@
package com.github.unchama.seichiassist.subsystems.breakskilltriggerconfig.domain

case class BreakSkillTriggerConfig(config: Map[BreakSkillTriggerConfigKey, Boolean]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

914bf47 の変更で、この subsystem は「マナがなくなった時に採掘機能を停止するか」を扱うと決めたので、この case class では「設定が有効か無効か」を保持できれば十分だと思います。

なので

Suggested change
case class BreakSkillTriggerConfig(config: Map[BreakSkillTriggerConfigKey, Boolean]) {
case class BreakSuppressionPreference(doBreakSuppression: Boolean) {

とすると良いと思います。(ついでにファイル名も BreakSuppressionPreference.scala とすると良いと思います)

...hama/seichiassist/subsystems/breakskilltriggerconfig/domain/BreakSkillTriggerConfigKey.scala
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BreakSkillTriggerConfig に対して書いたレビューに沿った変更が行われると、BreakSkillTriggerConfigKey の定義が不要になると思います。

...bsystems/breakskilltriggerconfig/infrastructure/JdbcBreakSkillTriggerConfigPersistence.scala
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SQL 周りも現在のテーブルスキーマに合わせた実装に変更をお願いします！

src/main/scala/com/github/unchama/seichiassist/subsystems/mana/domain/ManaAmount.scala
Comment on lines +15 to +21
def tryConsume(amount: ManaAmount)(manaMultiplier: ManaMultiplier): Option[ManaAmount] = {
val resultingAmount = value
Option.when(resultingAmount >= amount.multiply(manaMultiplier.value).value)(
ManaAmount(resultingAmount)
)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tryConsume 関数を新たに定義する必要はないかもしれません。

この関数を使用しているのは canAcquire 関数ですが、canAcquire 関数は「任意の分だけマナを消費することができるか」を確認する関数なので、tryUse 関数を呼び出すことで必要な要件を満たせると思います
(tryUse 関数は消費したいマナに manaMultiplier をかけ合わせた分を、現在のマナから引いて、負の値になるならば None を返す関数なので)

src/main/scala/com/github/unchama/seichiassist/subsystems/mana/domain/ManaManipulation.scala
Comment on lines +63 to +70
dragonNightTimeMultiplierRef.get.flatMap { multiplier =>
ref.modify { original =>
original.tryConsume(amount)(multiplier) match {
case Some(reduced) => (reduced, true)
case None => (original, false)
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

canAcquire 関数は「amount だけマナを消費することが可能ならば true、そうでないなら false を返す作用」なので、ref.modify を実行してしまうとまずいです(ref の値を更新してしまっているので)

なので

Suggested change
dragonNightTimeMultiplierRef.get.flatMap { multiplier =>
ref.modify { original =>
original.tryConsume(amount)(multiplier) match {
case Some(reduced) => (reduced, true)
case None => (original, false)
}
}
}
for {
multiplier <- dragonNightTimeMultiplierRef.get
original <- ref.get
usedResult <- original.tryUse(amount)(multiplier)
} yield usedResult.isDefined

とすると良いと思います。

...ain/scala/com/github/unchama/seichiassist/subsystems/mana/domain/LevelCappedManaAmount.scala
Comment on lines +25 to +30
def tryConsume(
amount: ManaAmount
)(manaMultiplier: ManaMultiplier): Option[LevelCappedManaAmount] = {
manaAmount.tryConsume(amount)(manaMultiplier).map(LevelCappedManaAmount(_, level))
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ManaAmount.scala の tryConsume 関数に対して書いたレビューと同様なので理由は省略しますが、tryConsume 関数は必要ないと思います。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rito528 rito528 rito528 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kuroma6666 @rito528